Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrapping up interceptors, fixing typos, removing Moq #2141

Merged
merged 25 commits into from
Apr 5, 2024

Conversation

alexeyzimarev
Copy link
Member

  • Make interceptors collection immutable in read-only options
  • Adding interceptors to request level, making old callbacks obsolete
  • Refactoring tests to not use Moq
  • Moving OnBeforeDerialization to the right place
  • Aligning interceptors:
    • Before request
    • Before HTTP request
    • After HTTP request
    • After request
    • Before deserialization

@alexeyzimarev
Copy link
Member Author

@fseidl-bauradar you might want to take a look

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 14, 2023

Deploying restsharp with  Cloudflare Pages  Cloudflare Pages

Latest commit: b80b25f
Status: ✅  Deploy successful!
Preview URL: https://8709975c.restsharp.pages.dev
Branch Preview URL: https://wrapping-up-interceptors.restsharp.pages.dev

View logs

@alexeyzimarev
Copy link
Member Author

I am not sure if throwing in interceptors should crash the request, like it's expected in tests. The current OnXXX hooks can produce exceptions, and if that results in client throwing depends on throw options. I want to replace the current hooks with interceptors, so I think they should express the same behaviour.

@fseidl-bauradar
Copy link
Contributor

I am not sure if throwing in interceptors should crash the request, like it's expected in tests. The current OnXXX hooks can produce exceptions, and if that results in client throwing depends on throw options. I want to replace the current hooks with interceptors, so I think they should express the same behaviour.

In my project, I am currently using it to convert a ErrorDTO from the DTO into a Exception.
Because my Application is currently in a HybridMode State, between using REST and direct DatabaseAccess.
But as I think the behaviour that a Exception in the Interceptors cancels the Request, sounds logical.
In Special when you are adding CancelationTokens to the interceptors

@alexeyzimarev
Copy link
Member Author

Yeah, I didn't think that interceptors can't cancel the request, but the way it work now with hooks is that the ExecuteAsync doesn't throw unless ThrowOnAnyError is set to true and you can an error response back where the RestResponse.ErrorException is set to the produced exception.

@fseidl-bauradar
Copy link
Contributor

fseidl-bauradar commented Sep 19, 2023

Yeah, I didn't think that interceptors can't cancel the request, but the way it work now with hooks is that the ExecuteAsync doesn't throw unless ThrowOnAnyError is set to true and you can an error response back where the RestResponse.ErrorException is set to the produced exception.

Sorry, but your answer is a little bit confusing,
As I understand it you want to say that interceptors can't cancel the request, but the way it work now with hooks is that the ExecuteAsync doesn't throw unless ThrowOnAnyError is set to true and a developer get a resppnse back where the RestResponse.ErrorException is set to the produces exception.

Am I understanding you right?
One possible usage of an interceptor, would be a validation of the data before sending it to the Server, avoiding useless roundtrips.
Is this possible using your implementation?

@fseidl-bauradar
Copy link
Contributor

fseidl-bauradar commented Mar 15, 2024

Have you found the reason for the build errors on linux and Claudflare Pages?
Would be nice if I could use the official package again and don't use my self hosted version for ever.

@alexeyzimarev alexeyzimarev merged commit d758e40 into dev Apr 5, 2024
11 checks passed
@repo-ranger repo-ranger bot deleted the wrapping-up-interceptors branch April 5, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants